Spring Boot migration - AAD related samples#10614
Conversation
saragluna
requested review from
JonathanGiles,
anuchandy,
hemanttanwar,
joshfree,
mitchdenny and
srnagar
as code owners
| <version>2.2.5-beta.1</version> <!-- {x-version-update;com.microsoft.azure:azure-active-directory-spring-boot-starter;current} --> | ||
| </dependency> | ||
| <!-- | ||
| <dependency> |
There was a problem hiding this comment.
Why this is commented out, if this is not needed, should we remove it ?
There was a problem hiding this comment.
The commented out parts are in following PRs.
| In Azure portal, app registration manifest page, configure `oauth2AllowImplicitFlow` in your application manifest to `true`. See [this issue](https://github.com/MicrosoftDocs/azure-docs/issues/8121#issuecomment-387090099) for details on this workaround. | ||
|
|
||
| ## Next steps | ||
| ## Contributing No newline at end of file |
There was a problem hiding this comment.
We should put standard text for contributing.
There was a problem hiding this comment.
This contributing guide is very specific to azure-sdk-for-java and its related tools. You probably want a guide which talks about how user can contribute to something which is specific to azure-spring-*
JimSuplizio
left a comment
JimSuplizio
left a comment
There was a problem hiding this comment.
There are some guidelines that were written for spring which need to be followed.
The spring libraries are either track 1 (data) or track 2 (client). The way things are being declared is a hybrid, the libraries themselves are com.microsoft.azure and have their versions in version_data.txt which implies data however the sdk/spring/ci.yml is using the client template and the various libraries are using the azure-client-sdk-parent which isn't correct for a data track library. The differences in the parents does matter since it concerns what you do and do not need in the pom file. For example track 1 doesn't get an entry in jacoco nor is it subject to maven-enforcer-plugin.
| <artifactId>azure-sdk-template</artifactId> | ||
| <version>1.0.4-beta.13</version> <!-- {x-version-update;com.azure:azure-sdk-template;current} --> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
If this is a track 1, or data, library it does not belong in here. Jacoco is for track 2, or client libraries only.
There was a problem hiding this comment.
@JimSuplizio Thanks for the clarification. I'll define the versions of Spring modules in version_client.txt then. We'd like to use track 2 pipelines for Spring modules, but only with the group id azure.microsoft.com for as long as one more release for them.
JimSuplizio
left a comment
JimSuplizio
left a comment
There was a problem hiding this comment.
This is the only PR, out of the set of spring PRs, that I'm not creating issues for and just approving. This is because of the BOM files. In the email that went out last week I pretty clear on BOM files:
BOM files are not used in the repository for building libraries within the repository. If Spring needs to define a BOM file, for consumers, then it should be done in a manner similar to the Azure SDK BOM for client libraries and should be in the sdk/boms directory.
I'm also under the impression that BOM files aren't supposed to be frequent releases as such, the SDK libraries in BOM files shouldn't be tagged with 'current', those versions are carefully updated by hand. Right now the BOM is inside the spring directory and being used to build the samples. If this is going to change in one way or the other I'd like to hear input/thoughts from @mitchdenny and @JonathanGiles .
|
I agree with @JimSuplizio on BOMs. We should not create a BOM with the intention of referencing it as an internal dependency (that is, our Spring Boot libraries depend on it). If you choose to create a BOM then it will need to go in |
|
@mitchdenny @JimSuplizio We're going to apply such changes, moving spring-bom to sdk/boms folder and removing dependencyManagement sections. |
remove dependencyManagement and move bom file to sdk/boms
|
/azp run java - aggregate-reports |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@hemanttanwar @JimSuplizio Please help approve this PR. The master's been rebased and aggregate-reports run. |
JimSuplizio
left a comment
JimSuplizio
left a comment
There was a problem hiding this comment.
@saragluna I'm going to pre-approve this however the spring BOM file should not be using the current or dependency tags (external dependency is okay) of the spring libraries. The BOM files versions should get updated before it's released which is supposed to be much less frequently than the library releases.
Thanks for pointing it out, I've removed all current tags from the BOM file. |
|
/azp run java - aggregate-reports |
|
Azure Pipelines successfully started running 1 pipeline(s). |
5 participants
Split PR #9395 into several PRs for the convenience of reviewing.
Modules included: